-
Notifications
You must be signed in to change notification settings - Fork 0
Sprint 8 solution time and duration #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
* src/manager/InMemoryHistoryManager.java
+ Реализован двусвязный список + HashMap<id,Node>
+ add(task) теперь удаляет дубликаты через removeNode за O(1)
+ remove(id) — новый метод HistoryManager
* src/manager/TaskManager.java
+ в интерфейс добавлен void remove(int id)
* src/manager/InMemoryTaskManager.java
+ вызовы historyManager.remove(...) в removeTask / removeEpic / removeSubtask
* src/manager/HistoryManager.java
+ объявлен метод remove(int id)
* tests
+ HistoryManagerTest — проверяет отсутствие дублей и порядок
+ InMemoryTaskManagerTest — проверка очистки истории при удалении,
истории > 10, дубль-просмотров
+ TaskManagerHistoryIntegrationTest — интеграция: удаление эпика убирает
эпик и все Subtask'и из истории
Issue: sprint-6 / ТЗ «неограниченная история без дублей»
… методах, сохранение исходных id
…restore (если восстановление)
…а O(1); чистка импортов
…обы убрать предупреждения
… minimal IDEA files
PrioritizedViewTest:
LexLippi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Привет!
Неплохая работа!
Есть некоторое количество замечаний, которые необходимо исправить!
Желаю удачи!
src/manager/InMemoryTaskManager.java
Outdated
| // статус | ||
| if (subs.isEmpty()) { | ||
| epic.setStatus(Status.NEW); | ||
| } else { | ||
| boolean allNew = subs.stream().allMatch(s -> s.getStatus() == Status.NEW); | ||
| boolean allDone = subs.stream().allMatch(s -> s.getStatus() == Status.DONE); | ||
| if (allNew) { | ||
| epic.setStatus(Status.NEW); | ||
| } else if (allDone) { | ||
| epic.setStatus(Status.DONE); | ||
| } else { | ||
| epic.setStatus(Status.IN_PROGRESS); | ||
| } | ||
| } | ||
|
|
||
| // duration = сумма минут (null считаем как 0) | ||
| long minutes = | ||
| subs.stream() | ||
| .map(Subtask::getDuration) | ||
| .filter(Objects::nonNull) | ||
| .mapToLong(Duration::toMinutes) | ||
| .sum(); | ||
| epic.setCalculatedDuration(minutes == 0 ? null : Duration.ofMinutes(minutes)); | ||
|
|
||
| // start = минимальный start subtask; end = макс end | ||
| LocalDateTime start = | ||
| subs.stream() | ||
| .map(Subtask::getStartTime) | ||
| .filter(Objects::nonNull) | ||
| .min(LocalDateTime::compareTo) | ||
| .orElse(null); | ||
|
|
||
| LocalDateTime end = | ||
| subs.stream() | ||
| .map(Subtask::getEndTime) | ||
| .filter(Objects::nonNull) | ||
| .max(LocalDateTime::compareTo) | ||
| .orElse(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Делаем 5 пробеганий по коллекции подзадач, кажется, что все эти действия можно выполнить за один обход
| /* ───────────── CSV утилиты ───────────── */ | ||
|
|
||
| private static Task fromCsv(String csv) { | ||
| String[] p = csv.split(",", -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
лучше не называть переменные одним символом. taskParts вполне подойдет
| } | ||
| } | ||
|
|
||
| private static LocalDateTime parseTimeOrNull(String s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это не имеет отношения к FileBackedTaskManager, поэтому лучше унести в отдельный класс с аналогичными утилитами
| int maxId = 0; | ||
| for (Task task : tasks) { | ||
| maxId = Math.max(maxId, task.getId()); | ||
| } | ||
| for (Epic epic : epics) { | ||
| maxId = Math.max(maxId, epic.getId()); | ||
| } | ||
| for (Subtask subtask : subtasks) { | ||
| maxId = Math.max(maxId, subtask.getId()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем делать второй проход по коллекциям, если идентификаторы не меняем?
src/model/Epic.java
Outdated
| public void setCalculatedDuration(Duration duration) { | ||
| this.duration = duration; | ||
| } | ||
|
|
||
| public void setCalculatedStart(LocalDateTime start) { | ||
| this.startTime = start; | ||
| } | ||
|
|
||
| public void setCalculatedEnd(LocalDateTime end) { | ||
| this.endTime = end; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Эти методы лучше не делать public иначе их может нечаянно вызвать в коде другой программист, что приведет к нарушению логики работы.
Поэтому публичные методы должны быть действительно публичными
src/model/Epic.java
Outdated
| private static String escape(String s) { | ||
| return s == null ? "" : s; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Метод лучше вынести в отдельный класс с утилитами
src/model/Subtask.java
Outdated
| private static String escape(String s) { | ||
| return s == null ? "" : s; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Аналогично про этот метод и не придется его дублировать
LexLippi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Привет!
Хорошая работа!
Желаю удачи в следующих спринтах!
Спасибо за ревью! Исправил замечания:
Пересчёт эпика за один проход:
– добавлен Epic.recalcFromSubtasks(List),
– InMemoryTaskManager.recalcEpic(...) теперь вызывает пересчёт эпика (без 5 проходов).
Нейминг:
– в FileBackedTaskManager.fromCsv(...) p → taskParts.
Утилиты:
– parseTimeOrNull и escape вынесены в util.CsvUtils;
– дублирующие escape(...) в моделях удалены; в Task/Epic/Subtask теперь CsvUtils.escape(...).
Восстановление из файла:
– maxId считаю на лету при чтении строк CSV (без дополнительных проходов по коллекциям).
Расчётные поля Epic:
– публичные сеттеры расчётных полей убраны; вместо этого публичный recalcFromSubtasks(...).
Дополнительно:
– добавлены TODO-комментарии в местах правок по ревью.
Что сделано по ТЗ
Модель
Task
добавлены поля Duration duration и LocalDateTime startTime;
вычисляемый getEndTime() как startTime + duration;
toCsvRow() расширен—добавлены колонки durationMinutes и startTime;
equals/hashCode/toString учитывают новые поля.
Subtask
наследует duration/startTime/getEndTime;
хранит epicId; корректный экспорт в CSV.
Epic
расчётные поля: duration, startTime, endTime считаются от подзадач;
пакетные сеттеры setCalculatedDuration/Start/End для пересчёта менеджером;
хранит subtaskIds.
Менеджеры
InMemoryTaskManager
приоритизация через TreeSet по startTime (доп. сравнение по id);
в приоритизированный список не попадают эпики и задачи без startTime (по ТЗ);
валидация пересечений при add/update задач и подзадач; пересечение определяется как наложение интервалов [start, end), касание по границе допустимо;
пересчёт эпика:
status (все NEW → NEW; все DONE → DONE; иначе IN_PROGRESS),
duration = сумма минут подзадач,
start = минимальный startTime подзадач, end = максимальный endTime;
методы для восстановления из файла: putTask/Epic/SubtaskPreserveId и setNextIdAfterRestore.
InMemoryHistoryManager
история просмотров как LinkedHashMap с дедупликацией;
лимит 10 элементов; поддержка remove(id).
Файловый менеджер
FileBackedTaskManager
сохранение в CSV в расширенном формате:
id,type,name,status,description,durationMinutes,startTime,epic;
обратная совместимость чтения старого формата (6 колонок из спринта 7);
восстановление: чтение всех строк → разбор в Task/Epic/Subtask → put*PreserveId → setNextIdAfterRestore;
автосохранение на всех add/update/remove.
формат времени единый — yyyy-MM-dd HH:mm (как в Task.CSV_TIME_FMT).
Тесты
TaskManagerTest — общие сценарии:
правила статуса эпика (без сабтасков, все NEW, все DONE, смешанные/IN_PROGRESS);
приоритизация + запрет пересечений;
агрегирование времени эпика (start, end, duration).
InMemoryTaskManagerTest и FileBackedTaskManagerTest — проверки для обеих реализаций (вторая работает с temp-файлом и корректно чистит его).
InMemoryHistoryManagerTest — дедуп, лимит 10, удаление.
EpicStatusTest — быстрый санити-тест правил статуса на обеих реализациях.
Нефункциональные
привёл код к Google Java Style (фигурные скобки в if/while/for, отступы, переносы);
устранил/подавил безопасные IDE-варнинги (@SuppressWarnings там, где метод нужен публично, но прямо сейчас не вызывается; аккуратный delete() в тестах на Windows).
Формат CSV (итог)
id,type,name,status,description,durationMinutes,startTime,epic
durationMinutes — целое число минут или пусто;
startTime — yyyy-MM-dd HH:mm или пусто;
у Task/Epic поле epic пустое; у Subtask — epicId.
Ограничения/договорённости
эпики не включаются в getPrioritizedTasks();
задачи без startTime не участвуют в приоритизации;
касание интервалов по границе не считается пересечением.
Как проверить
запустить тесты из IDE ;
руками: создать задачи/подзадачи с startTime и duration, убедиться в сортировке, запрете пересечений и корректности истории;
для файлового менеджера: создать/перезапустить — данные сохраняются/восстанавливаются из CSV